First of all, thanks much for the contribution! :)

I just checked the official documentation to verify the suggested changes, and it looks mostly good to me. However, I have a few questions and suggestions which I'd like you to consider before I could merge this:

1. I found that some of the properties of MdDialogOptions are not wrapped with UndefOr, unlike others. Is this intentional? If so, could you elaborate on what criteria did you use in determining whether or not a certain property to be wrapped as UndefOr[T]?
2. Is there any reason to declare MdDialogOptions as ScalaJSDefined, while making others as JS native traits?
3. It seems that you prefer to declare properties of an option object as vals, instead of vars. In fact, I haven't decided myself between these alternatives - making option objects as immutable seems to be in line with Scala conventions, but it would also make it mandatory to duplicate every properties as arguments in the apply() method.

Personally, I'm beginning to change my mind in favor of using immutable options. However, I'm a bit concerned if it would confuse the users if we apply such a convention to this service only, without changing existing codebase as well. So, I hope you could share your opinion about this issue.

1. How can we instantiate MdDialogOptions? Shouldn't it have a factory object, like the case with MdDialogOptions?

And I have a few suggestions as well:

1. It seems there are a few places where we could use union type arguments, like MdDialogOptions.openFrom, which can accept any of string, Element, or object type value according to the documentation. We don't have to choose only one of them, as such a case can be nicely expressed using the new 'union type' in Scala.js.
2. You can use scala.scalajs.js.undefined inistead of defining it as ().asInstanceOf[js.UndefOr[Nothing]].
3. It's a minor one, but it would be nice if you could reformat and reorganize the source code according to the existing convention. For instance, UndefOr is referenced without js. prefix in other part of the codebase, and we have spaces inside parenthesis used in a import statement, and etc.
4. Also a minor one, but we can use org.scalajs.dom.ClientRect instead of org.scalajs.dom.raw.ClientRect.

By the way, sorry for the late response, and thanks again for the contribution!

I am trying to implement your suggestions, but I'm having an issue. I have never used the union types before. If I say:

@js.native
trait MdDialogOptions extends js.Object {
  val openFrom: UndefOr[String | Element | js.Object]
}
object MdDialogOptions {
  def apply(
    openFrom: UndefOr[String | Element | js.Object] = undefined
  ) =
    js.Dynamic.literal(
      "openFrom" -> openFrom
    ).asInstanceOf[MdDialogOptions]
}

Then the compiler complains:

[error] /usr/local/src/scalajs-angular/src/main/scala/com/greencatsoft/angularjs/extensions/material/MdDialog.scala:139: type mismatch;
[error]  found   : scala.scalajs.js.UndefOr[scala.scalajs.js.|[scala.scalajs.js.|[String,org.scalajs.dom.Element],scala.scalajs.js.Object]]
[error]     (which expands to)  scala.scalajs.js.UndefOr[scala.scalajs.js.|[scala.scalajs.js    [String,org.scalajs.dom.raw.Element],scala.scalajs.js.Object]]
[error]  required: scala.scalajs.js.Any
[error]       "openFrom" -> openFrom,
[error]                     ^

The definition of the factory method for Dynamic.literal seems to say it will take a Scala.Any, so I am confused why it is demanding a scalajs.js.Any.

def applyDynamic(name: String)(
    fields: (String, Any)*): Object with Dynamic = sys.error("stub")

Can you see what I'm doing wrong?

I'm terribly sorry, but I just realized that I missed your last message somehow. And I was wondering if you have abandoned this PR... Urg.

Anyway, to answer you question, I think the Any in question is actually scalajs.js.Any, not scala.Any. js.Dynamic resides in the same package as the former so it has the precedence over the latter.

On a side note, I guess we can avoid using UndefOr in the apply method if you think it'd make the API less confusing, by adding overloaded method without arguments.

Again, sorry for my missing your post and I hope to hear from you again.

No worries. It was my fault since I updated an existing comment rather than posting a new one, so the system deemed it unnecessary to send you an email. I was hoping I would come up with a perfect solution before you noticed, but my hope is unrealized.

You are correct about the parameter type for Dynamic.literal() but I still lack understanding why union types fail to pass as scala.js.Any. The parameters in question are MdDialogOptions.openFrom, closeTo and controller. For example, none of the following work:

openFrom: UndefOr[String | Element | ClientRect] = undefined,
openFrom: UndefOr[String | ClientRect] = undefined,
openFrom: Element | ClientRect,

If you look in my current proposed version of MdDialog.scala line 120 you'll see I have it as UndefOr[String] and in commented-out lines directly above are what I tried unsuccessfully.

Another issue is the controller parameter, since that is supposed to be able to take either a String or a Controller, but our Controller is not a js.Any. Do you have an idea of how to deal with that? Anyway, the union-type issue seems to preclude resolution of this issue.

As for overloading, that seems impractical for the MdDialogOptions factory method since it has so many parameters. I would prefer to wrap each parameter in UndefOr.

But again, the problem with union-types being recognized as scala.js.Any is where I am stuck.

I finally had some time to investigate this issue. I found that the union operator(|) is actually a trait, so the type String | Element translates to |[String, Element] which does not extend from js.Any, thus not applicable to the method in question.

As to the controller problem, I suppose we have to use js.Any for now, if we are to allow assigning an actual underlying controller instance to the option.

Controller.proxy[A] creates and returns such an instance which can be used here. I admit that it's a bit ugly, but should at least the solve the problem for now.

Again, sorry for taking much time to write a reply. I'll try to merge this PR as soon as you could update it this time. Thanks for the patience!

As to the controller problem, I suppose we have to use js.Any for now, if we are to allow assigning an actual underlying controller instance to the option.

Controller.proxy[A] creates and returns such an instance which can be used here. I admit that it's a bit ugly, but should at least the solve the problem for now.

Okay, so if I want to invoke proxy on an argument if and only if it is a Controller, then how do I determine whether it is a Controller or a String?

My first thought is something like this:

def apply(
  controller: UndefOr[Controller[_]|String] = undefined
) = {
  js.Dynamic.literal(
    "controller" -> controller.map { _ match {
      case c: Controller[_] => Controller.proxy(c)
      case s: String => s
    }  }
  ).asInstanceOf[MdDialogOptions]
}

But that fails with warnings and errors. Do you know how to determine which of its alternatives (Controller or String) a union type is?

First of all, I'd like to correct my mistake when I said Controller[A].proxy creates an actual instance of Angular controller, which is not the case.

What is returned by that method (and which is also the type of Directive.controller method) is a Javascript array, which functions as a service definition object to be used by the Angular module.

By the way, I'm trying to refactor the relevant API now (c91c8e5), so we might need to wait before the proposed changes to be merged first.

With the changes, now we have ServiceDefinition[A <: Controller[_]] which we can use for such a case (like this one) where an option object expects a controller object like,

  def apply(controller: ServiceDefinition[_ <: Controller[_]]): MdDialogOptions

And you will be able to create a service definition object with ServiceDefinition[MyController] or ServiceDefinition(MySingletonController).

I suppose we can probably construct a union type between String and ServiceDefinition[_], but using method overloading is always an option, in case it doesn't work.

I'll merge the other PR as soon as I get an approval from the original contributor.

Just merged the branch which contains the above mentioned enhancement. Please let me know if you need any help integrating the changes.